fix: correct ProviderValue typing, conftest docstring placement, and pool.py line formatting#22
Conversation
|
|
…ng, pool.py format Co-authored-by: mahimairaja <81288263+mahimairaja@users.noreply.github.com> Agent-Logs-Url: https://github.com/mahimairaja/openrtc-python/sessions/8aca67d0-da08-4745-899f-296a8db94b8b
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## cursor/split-cli-app #22 +/- ##
=====================================================
Coverage 90.22% 90.22%
=====================================================
Files 13 13
Lines 1361 1361
=====================================================
Hits 1228 1228
Misses 133 133
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses three code-quality issues: improving the usefulness of ProviderValue typing, ensuring the tests/conftest.py module docstring is recognized correctly, and aligning pool.py formatting with ruff format.
Changes:
- Adjust
ProviderValuetype alias inprovider_types.pyand remove the unusedAnyimport. - Move
tests/conftest.pymodule docstring above thefrom __future__ import annotationsstatement. - Apply
ruff formatoutput topool.py(line-length compliance).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/conftest.py |
Moves module docstring to proper location so it is treated as __doc__. |
src/openrtc/provider_types.py |
Updates ProviderValue alias and cleans up typing imports. |
src/openrtc/pool.py |
Formatting-only change to satisfy ruff format --check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ``object`` allows any third-party plugin class without enumerating them here; | ||
| # use strings when you want the type checker to stay precise. | ||
| ProviderValue: TypeAlias = str | object |
There was a problem hiding this comment.
str | object will be simplified by mypy to just object (since object is a supertype of str), so this alias still loses the intended “string vs provider object” precision. Consider either making the alias simply object and updating the comment accordingly, or introducing a narrower type/Protocol for provider instances so the union remains meaningful for type checking (and so ProviderValue | None doesn’t collapse to object).
Three code quality issues flagged in review: a type alias that was effectively
Any, a module docstring that wasn't being recognized as such, and a formatter compliance issue.Changes
provider_types.py—str | Anycollapses toAnyin all type checkers, makingProviderValueuseless for precision. Changed tostr | object; removed now-unusedAnyimport.tests/conftest.py— Module docstring was placed afterfrom __future__ import annotations, making it an unreachable string expression rather than__doc__. Moved above the future import.pool.py— Ranruff format; theifcondition at line 631 is exactly 88 chars (at the configuredline-length), so the formatter keeps it single-line andruff format --checknow passes cleanly.🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.